Hi @michaelmackenzie,
You have proposed changes to files in these packages:

• DAQ

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for 9649ba9: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

☔ The build tests failed for 9649ba9.

N.B. These results were obtained from a build of this Pull Request at 9649ba9 after being merged into the base branch at b4e91ff.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

Let me review the actual code changes in this PR.Let me get the actual file contents from the PR branch to review the code changes.Here is my review of PR #1775. I've identified the following issues across the 4 changed files:

StrawDigisToFragments_module.cc — New Module (501 lines)

1. Wrong namespace — module declared in art:: instead of mu2e::

The module class is declared as art::StrawDigisToFragments (lines 133–137). All other Mu2e modules in this codebase (e.g., mu2e::StrawDigisFromArtdaqFragments) live in the mu2e namespace. Placing a user module in the art namespace is unconventional and could cause issues with name collisions in the art framework.

2. TrackerDataPacket mainPacket in TrackerHitPayload is not zero-initialized

Line 100 constructs TrackerHitPayload payload; — the mainPacket member is a POD-like struct (TrackerDataPacket). Only some fields are explicitly set afterward (lines 101–109). Any padding bytes or bitfield bits not explicitly assigned will contain indeterminate values, which could lead to non-reproducible fragments. Consider zero-initializing: TrackerHitPayload payload{}; std::memset(&payload.mainPacket, 0, sizeof(TrackerDataPacket));

3. transferByteCount uses uint16_t — risk of overflow for large hit counts

Line 324: block.transferByteCount = static_cast<uint16_t>((numPackets + 1) * 16);. With many hits per ROC link, (numPackets + 1) * 16 can exceed 65535 and silently wrap. The DTC_DataBlock constructor takes a uint16_t, so this may be an upstream constraint, but a range check or at least a warning would be prudent.

4. skipFragmentOnSizeMismatch_ config parameter is declared but never used

The skipFragmentOnSizeMismatch fhicl parameter is declared (line 141), stored as a member (line 169), and initialized (line 193), but is never referenced anywhere in the module logic. This appears to be dead code / an unfinished feature.

5. Inconsistent indentation in constructor initializer list

Lines 194 and 197–198 use 2-space indent for initializer entries while the rest use 4-space. Minor, but inconsistent with the surrounding code style:

    , skipFragmentOnSizeMismatch_(config().skipFragmentOnSizeMismatch())
  , fallbackToOfflineWhenMapMissing_(config().fallbackToOfflineWhenMapMissing())   // <-- 2-space
    , forceOfflineAddressing_(config().forceOfflineAddressing())                     // <-- 4-space
    , strawDigiTag_(config().strawDigiTag())
  , strawDigiADCTag_(config().strawDigiADCTag())                                    // <-- 2-space

6. Empty ROC blocks emitted for links with zero hits

The inner loop at line 309 (for (uint8_t linkID = 0; linkID < kTrackerRocsPerDtc; ++linkID)) iterates over all 6 links and calls linkMap[linkID], which default-constructs empty TrackerBlockData entries via std::map::operator[]. This emits a 16-byte header-only data block for every unused link. While this may be intentional for decoder compatibility (as the comment suggests), it inflates fragment sizes and may confuse downstream tools that don't expect zero-hit ROC blocks.

7. waveformPacketCount may silently truncate trailing ADC samples

Line 93: (samples.size() - 3) / 12 uses integer division, so if the waveform has e.g. 18 samples (3 + 15), only 1 ADC packet is created, and the last 3 samples are silently dropped. There's no diagnostic for this truncation.

8. Prefer std::make_unique over raw new

Line 382: std::unique_ptr<artdaq::Fragments> fragments(new artdaq::Fragments()); should use auto fragments = std::make_unique<artdaq::Fragments>(); per modern C++ best practice.

9. total_events_ and total_digis_ are not initialized in-class

Lines 178–179 declare long int total_events_; long int total_digis_; without default initializers. They're set to 0 in the constructor body (lines 200–201) rather than in the initializer list or via in-class defaults, which is fragile.

StrawDigisFromArtdaqFragments_module.cc — Existing Module Changes

10. Indentation is broken on the StrawId sid construction

Lines 501–503 in the diff show the new StrawId sid line indented with 18 spaces (deep nesting) while the surrounding code uses a different indent level. The original code had sid at column 14, and the replacement shifts it right inconsistently.

11. Potential null-pointer dereference on tpm in mnid consistency check

Line ~460 (in the original): after the keyOnMnid_ block, if allowOfflineFallbackWhenPanelMapMissing_ is true and tpm is null, execution continues. But the original code at line ~482 checks tpm->mnid() != mnid — the new guard !forceOfflineAddressing_ && tpm != nullptr at that line correctly prevents this, but the subsequent block at ~line 484–498 (the sid.straw() debug print path) also accesses tpm->uniquePlane() / tpm->panel(), which is now guarded by the new planeForPrint/panelForPrint variables. This is handled correctly — just noting the logic is delicate.

StrawDigiCollectionsComparator_module.cc — New Analyzer

12. Missing Offline::DataProducts from CMakeLists link dependencies

The comparator module includes Offline/DataProducts/inc/StrawEnd.hh but the CMakeLists.txt entry (lines 139–142) only links Offline::RecoDataProducts. It may work transitively, but explicitly listing Offline::DataProducts would be safer.

13. std::cout used instead of mf::LogInfo / mf::LogWarning

Both new modules use std::cout throughout for diagnostics. The Mu2e/art convention is to use the message facility (mf::LogInfo, mf::LogWarning, mf::LogError) for module output, which allows better log filtering and destination control. This applies across both new files.

Summary of Key Issues

Responding to the AI comments:

1. There are several DAQ modules in the art namespace (I'm not positive why), but I've moved this to our namespace
2. I've initialized these to 0 (but I believe these are all set in the code and the SetWaveform calls)
3. I don't think there should be 32 kB single straw digis/fragments, and I haven't seen an overflow in a few hundred thousand digis tested
4. This was inherited from previous testing, I've deleted it
5. Fixed the indentation
6. I believe this matches the DAQ and decoding assumptions
7. I assume this is true in the DAQ as the decoding validates this
8. Changed to this
9. They are already initialized in the constructor?
10. Fixed the indentation
11. Fixed
12. Mu2e uses cout in many places for diagnostic/debug printouts as well

@FNALbuild run build test

⌛ The following tests have been triggered for 40abec9: build (Build queue - API unavailable)

☀️ The build tests passed at 40abec9.

N.B. These results were obtained from a build of this Pull Request at 40abec9 after being merged into the base branch at 723b9b1.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.